Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Check Action Type #712

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Check Action Type #712

wants to merge 7 commits into from

Conversation

MentalGear
Copy link

@MentalGear MentalGear commented Feb 27, 2020 •
edited
Loading

#707

Description

TypeError: 'int' object is not subscriptable
Ran a couple time into this bug, and found it really hard to debug within stable baselines.
To spare others (as well as my future self) much frustration I would suggest to add a type check to wrapper envs like dummyEnv (or only check_env?) before using the action to make sure it's an iterable, before continuing with the code.

The following assertion will give a developer friendly error message that's easy to understand and offers an immediate solution, saving much frustration.

Motivation and Context

#707

  • I have raised an issue to propose this change

closes #707

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I've read the CONTRIBUTION guide (required)
  • I have updated the changelog accordingly (required).
  • My change requires a change to the documentation.
  • I have updated the tests accordingly (required for a bug fix or a new feature).
  • I have updated the documentation accordingly.
  • I have ensured pytest and pytype both pass (by running make pytest and make type).

@MentalGear MentalGear marked this pull request as ready for review February 27, 2020 22:15
@araffin
Copy link
Collaborator

araffin commented Feb 28, 2020

Hello,
thanks for the PR, please don't forget to add a test, and add your username to the changelog.
note that some tests are failing because of an internal wrapper (unvecwrapper).

@MentalGear
Copy link
Author

OK, added tests. I'm not sure though if it's the right approach to check each step of the base env if the action type received is compatible, as it seems a bit of unnecessary overhead. Maybe just check on the first run ?

@araffin araffin mentioned this pull request May 12, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Action Type Check in Env [feature request]
2 participants
@MentalGear @araffin